-
Notifications
You must be signed in to change notification settings - Fork 543
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
node: store callbacks in map #3772
base: master
Are you sure you want to change the base?
Conversation
For #3706 ? No, I think the callbacks need to be stored in the instance itself, as properties. We could have a "__callbacks" property that points to an Array, but that would be externally accessible (bad). The solution to having these kind of "private" properties is to use Symbols as key, instead of a string. |
Yes I mean this issue. I have done an commit. Do mean more something more like this? |
Yes, that looks like a good direction. The main issue I have is that this only works for the wrapper. If we make the interpreter public, then users of that will still have memory leaks. Does it pass the test? If yes, can you add this also to the CI? |
Yes that's true it won't fix it for the Yes it works with the tests. |
I don't think this will work. the We need to break the cycle somehow. I'm not familiar about napi, but could this help us? https://docs.rs/napi/latest/napi/bindgen_prelude/struct.WeakReference.html native component -> array -> user js function -> instance -> native component. And that cycle is ok because everything is managed by the garbage collector, there is no root there. (while the RefCountedReference introduces a root) |
Florian's patch does indeed create a cyclic reference, because it keeps a strong reference to the (new) closure that however still captures the outer environments that hold the instance. I'm convinced that the Symbol approach is the right way to create a "private" property. I do agree that this needs to be solved in .rs. What I'd do is (in Rust code, using napi API): (1) Create a JS array that has N entries where N is the number of callbacks (it's a fixed number). Every callback shall have a corresponding index in the array. That way the JS GC has a full view over the cyclic reference and can collect appropriately. |
@tronical does this go in the direction you have suggested?